Downgrade minimal required python to 3.11#51
Conversation
Requiring python>=3.12 is a bit of a burden. It's only required for `find_bids_datasets`, which uses `Path.walk`. Instead, downgrade minimum python to 3.11 and add a guard on this function in case python<3.12. Note, we could look into downgrading further. The next block would be `get_column_names` returns a `StrEnum`, which was introduced in 3.11. I'm hesitant to remove this though, because being able to treat these enum fields as native strings is nice.
There was a problem hiding this comment.
Pull Request Overview
This PR downgrades the overall minimum Python requirement to 3.11 while retaining Python 3.12–specific functionality for the find command via runtime guards. Key changes include:
- Updating the Python version requirement in pyproject.toml, README.md, and bids2table/init.py.
- Adding runtime guards in bids2table/_indexing.py and bids2table/main.py for find_bids_datasets.
- Adjusting tests in test_main.py and test_indexing.py to conditionally skip find-related tests and update expected outcomes.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_main.py | Removed direct calls to find command and added a guarded test. |
| tests/test_indexing.py | Introduced skipif for find tests and updated expected table size. |
| pyproject.toml | Lowered the Python requirement from >=3.12 to >=3.11. |
| bids2table/_indexing.py | Added a guard in find_bids_datasets to raise a runtime error for Python <3.12. |
| bids2table/main.py | Added a version check to error out when running the find command on Python <3.12. |
| bids2table/init.py | Updated badge and note to reflect the new minimal requirement and find command dependency. |
| README.md | Updated badge and note to align with the new version requirements. |
Comments suppressed due to low confidence (1)
tests/test_indexing.py:102
- The expected table length has been changed from 10133 to 9727. Please verify that the new expected value accurately reflects the intended behavior and data processed by the updated code.
assert len(table) == 9727
| bids-examples/eeg_cbm | ||
| ``` | ||
|
|
||
| > Note: `b2t2 find` requires python>=3.12. |
There was a problem hiding this comment.
[nitpick] While the project’s minimum Python version is now 3.11, the note implies that the find command requires Python >=3.12. Clarify in the documentation that only the find functionality is gated on Python 3.12, while the rest of the project supports Python 3.11.
| > Note: `b2t2 find` requires python>=3.12. | |
| > Note: While the overall project supports Python >=3.11, the `b2t2 find` command specifically requires Python >=3.12. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #51 +/- ##
==========================================
- Coverage 95.98% 95.41% -0.57%
==========================================
Files 6 6
Lines 423 436 +13
==========================================
+ Hits 406 416 +10
- Misses 17 20 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I would recommend not dropping old versions of dependencies any faster than https://scientific-python.org/specs/spec-0000/. |
|
Thanks for the link @effigies, that's a useful reference. So by that rule, it seems ok to not support 3.10, but indeed we should support 3.11. |
There was a problem hiding this comment.
Code-wise this largely looks good to me.
I did play around with this briefly to try to get s3 datasets working with py311, though had to change _indexing.py code code (namely using os.walk instead of path.walk) + type casting a little bit to get the tests passing. That said, I'm not sure what gets returned or if it actually works with b2t2 find - didn't dig too deep into this yet. Could leave this as a todo down the road + add in matrix for testing for supported py versions.
I've added the s3 support in py311 in #52.
- Enables use of `finds_bids_datasets` in py311. - `root` in `_indexing.py` is initially passed as original type, with walked `dirpath` typecasted as Path - Removed error for <py312 in throughout codebase + testing
- Only runs after formatting - Update to dependencies to include other versions of python support
`Path.walk` and `CloudPath.walk` depend on python>=3.12. Also, `CloudPath.walk` retrieves all files up front rather than iteratively. Here we add some directory walk logic of our own for iteratively finding BIDS datasets under a root directory.
|
I decided to just copy over the basic directory walk logic from @kaitj would you mind taking another look? |
kaitj
left a comment
kaitj
left a comment
There was a problem hiding this comment.
Looks good to me - feels faster to me, but maybe I am imagining it 😅
Had a few questions, but nothing blocking.
|
Thanks @kaitj! Will go ahead and merge. |
Requiring python>=3.12 is a bit of a burden. It's only required for
find_bids_datasets, which usesPath.walk. Instead, downgrade minimum python to 3.11 and add a guard on this function in case python<3.12.Note, we could look into downgrading further. The next block would be
get_column_namesreturns aStrEnum, which was introduced in 3.11. I'm hesitant to remove this though, because being able to treat these enum fields as native strings is nice.